fix ignoring packages that are imported after freezing #430
Conversation
pegler
force-pushed
the
late-import-ignores
branch
from
1c63e4c to
8e81aa3
Compare
pegler
force-pushed
the
late-import-ignores
branch
from
8e81aa3 to
8d913ac
Compare
|
hi @boxed and @spulec, thanks for your efforts in maintaining this package. We've found it incredibly useful, but unfortunately identified this bug. Right now we're patching freezegun to make it work correctly and it'd be great if we could get the fix applied to the package itself. Let me know if there are any changes you'd like to see. Thanks! |
|
Hi, Just ran into this because S3 was rejecting requests made from within It is easy to show that this is broken with a small patch against existing tests: And I came up with a similar fix: Would be great to have this fixed upstream 👍 Thanks! |
|
@immerrr I switched to https://github.com/adamchainz/time-machine a while ago and have been very happy. You need to take a slightly different approach to handling ignores, but it worked well for our project. |
| if _should_use_real_time(): | ||
| result = real_date.today() + cls._tz_offset() | ||
| else: | ||
| result = cls._date_to_freeze() + cls._tz_offset() |
There was a problem hiding this comment.
I don't understand what this added logic is for. cls._date_to_freeze() calls get_current_time() which already calls _should_use_real_time() and does this type of thing so this change seems redundant?
Line 174 in 92624e4
There was a problem hiding this comment.
@boxed I'm happy to submit my change which should be smaller if that would make it easier to get this in.
I could add the test case from this PR to make sure it covers the issue mentioned in this PR, too.
There was a problem hiding this comment.
@immerrr that looks wrong to me too honestly. It's too complex. Something is fishy.
There was a problem hiding this comment.
Just to make sure we are talking about the same thing, you mean my patch that touches these two conditionals is too complex, right?
There was a problem hiding this comment.
Yes. Correct. I feel like this logic should be fully contained within get_current_time itself. It just wouldn't be DRY otherwise, and it would also mean there's a ton of duplicate code and every place there isn't duplication would be a bug.
There was a problem hiding this comment.
There are also performance considerations here, e.g. some blocks like
if _should_use_real_time():
return call_some_builtin_function()
may work much faster than converting the result of get_current_time() back to the required format.
DRY is a valid principle, but repetition only manifests itself if the code needs to change. If it is a small module that is feature complete, what difference does it make if the code repeats one time, two times or ten times, the interpreter doesn't care.
That said, let me see if I can hide should_use_real_time inside get_current_time...
There was a problem hiding this comment.
may work much faster than converting the result of get_current_time() back to the required format.
There is no conversion in this code though.
That said, let me see if I can hide should_use_real_time inside get_current_time..
...but get_current_time already calls should_use_real_time! That's my point.
There was a problem hiding this comment.
There is no conversion in this code though.
There are conversions in functions like fake_monotonic whose "faking" versions use get_current_time
Lines 221 to 225 in be4127b
So if we continue with the DRY idea of moving _should_use_real_time inside get_current_time, functions like fake_monotonic will look like
def fake_monotonic():
current_time = get_current_time()
return (
calendar.timegm(current_time.timetuple()) +
current_time.microsecond / 1e6
)
which means they would take a performance hit even in cases when are using the "real" function. Do you have some other idea in mind?
but get_current_time already calls should_use_real_time!
I don't think it does, or maybe i'm looking somewhere else
Lines 169 to 170 in be4127b
|
@pegler that's cool! I've been meaning to try it, just haven't had time to check it out. As everyone here is painfully aware, neither me not spulec have the energy/motivation to maintain freezegun to the level it deserves as a very widely used library. So if time-machine is actually a viable replacement, that's very good to hear. |
When a package is imported after freezegun starts, ignore would not work correctly. This fixes
FakeDate.today(),FakeDatetime.now()andFakeDatetime.utcnow()to return the real date/datetime in ignored packages.fixes #420